Fix feed generator robustness at scale#1166
Open
simplysaru wants to merge 11 commits intodevelopfrom
Open
Conversation
Addresses three critical issues causing runaway scheduling and database bloat: 1. Cursor advancement before processing: Modified get_items_for_batch() to store the last batch ID in a pending property, only committing to persistent storage after successful batch processing in handle_batch_action(). This prevents timeouts from causing the cursor to advance before the batch is processed, which would cause retries to fetch the wrong batch and skip products. 2. Duplicate timeout retries: Added deduplication check in handle_unexpected_shutdown() using as_has_scheduled_action() before calling schedule_immediate(). This prevents overlapping retries that fetch different work than originally intended. 3. Missing circuit breaker: Added MAX_BATCHES_PER_CYCLE constant (1000) and batch counter to prevent runaway scheduling. The generator now aborts with a warning if the maximum batch limit is reached during a generation cycle. These fixes prevent the issues described in customer feedback where timeout errors create fan-out of duplicate batches, leading to excessive Action Scheduler entries and database performance issues on large catalogs. Fixes: PIN4WOO-70 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
github-actions
Bot
added
changelog: fix
Update test_handle_unexpected_shutdown_does_throttle_product_number_when_rescheduling_the_action to account for new deduplication behavior: - First timeout reschedules the action (expected behavior) - Second timeout skips rescheduling due to deduplication (action already exists) - Updated expectations to reflect that schedule_immediate is called once, not twice Add new test test_handle_unexpected_shutdown_skips_rescheduling_if_action_already_scheduled to specifically verify deduplication prevents overlapping retries. Also fix phpcs violations in test file. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The deduplication logic checks for existing actions using as_has_scheduled_action(), which queries the real Action Scheduler database. Mocked schedule_immediate calls don't register in that database, so we need to actually schedule duplicate actions using as_schedule_single_action() to properly test the deduplication behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Actions scheduled with time() are immediately eligible for processing, which may cause them to not be found by as_has_scheduled_action(). Schedule them 10 seconds in the future to ensure they remain in pending status for deduplication checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace mocked schedule_immediate with real Action Scheduler calls to properly test deduplication logic. The issue was that mocked calls don't register in the Action Scheduler database, so as_has_scheduled_action() couldn't find them. Now using anonymous class implementing ActionSchedulerInterface that wraps real as_schedule_single_action() calls, allowing deduplication checks to work correctly. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Anonymous classes may have compatibility issues with PHP 7.4. Created TestActionSchedulerProxy as a named class to ensure compatibility across all supported PHP versions (7.4-8.3). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove type hints from TestActionSchedulerProxy method signatures to match the ActionSchedulerInterface exactly. PHP 7.4 is stricter about signature compatibility when implementing interfaces. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add required function docblocks to satisfy WooCommerce coding standards. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The ActionSchedulerInterface::search() method has 3 parameters including . Added the missing parameter to match the interface signature. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Author
Test Status Update✅ Core Implementation CompleteAll three critical fixes have been successfully implemented in
✅ PHP Coding Standards: All PHPCS checks passing for PHP 7.4 and 8.3
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves robustness of the feed generation job chain under large catalogs by preventing cursor advancement on failed batches, deduplicating timeout retries, and adding a circuit breaker to avoid runaway Action Scheduler scheduling/database bloat.
Changes:
- Defer committing
feed_last_queued_item_iduntil after successful batch processing via a newpending_last_batch_id. - Deduplicate timeout retries in
handle_unexpected_shutdown()usingas_has_scheduled_action()to avoid overlapping reschedules. - Add a per-generation circuit breaker (
MAX_BATCHES_PER_CYCLE) tracked via persistentfeed_batch_count.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/FeedGenerator.php |
Implements deferred cursor commit, retry deduplication, and a batch-count circuit breaker. |
tests/Unit/FeedGeneratorTest.php |
Updates/extends unit tests around unexpected shutdown retry behavior and applies PHPCS-driven formatting changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1. Move deduplication check before throttling logic - Prevents unnecessary batch size adjustments when rescheduling is skipped - Only throttle when actually scheduling a retry 2. Reset pending_last_batch_id at start of handle_batch_action - Prevents stale cursor values from previous failed batches - Ensures deterministic state management across retries 3. Add test coverage for new behaviors - test_circuit_breaker_stops_processing_at_max_batches: Verifies MAX_BATCHES_PER_CYCLE limit - test_cursor_deferred_until_successful_batch_completion: Verifies cursor only advances after success Addresses Copilot review comments on PR #1166 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Author
Copilot Review Feedback Addressed ✅Thanks for the detailed review! I've addressed all three issues: 1. ✅ Moved Deduplication Check Before ThrottlingBefore: Throttling happened before checking for duplicates, causing unnecessary batch size adjustments. Benefit: Prevents repeatedly decreasing batch size on duplicate timeout events. 2. ✅ Reset Pending Batch ID for Deterministic StateBefore: Benefit: Ensures clean state for each batch attempt, preventing cursor position corruption. 3. ✅ Added Test Coverage for New BehaviorsAdded two new unit tests:
Benefit: Better test coverage for the core robustness fixes. All changes committed in: 164d3bf The implementation is now more robust and better tested. Happy to iterate further based on team feedback! |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Agent-Logs-Url: https://github.com/woocommerce/pinterest-for-woocommerce/sessions/235573f9-062b-4dfb-837b-1897c7d42237 Co-authored-by: simplysaru <2734132+simplysaru@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three critical issues in the feed generator that cause runaway scheduling and database bloat at scale, as reported in customer feedback.
Changes
Fix cursor advancement before processing
get_items_for_batch()to store the last batch ID in a pending propertyhandle_batch_action()Add deduplication for timeout retries
as_has_scheduled_action()check inhandle_unexpected_shutdown()before reschedulingAdd circuit breaker to prevent runaway scheduling
MAX_BATCHES_PER_CYCLEconstant (1000 batches)Problem
As reported in WordPress.org support thread, the feed generation job was not robust at scale:
feed_last_queued_item_id) advanced inget_items_for_batch()before batch processinghandle_unexpected_shutdown()blindly rescheduled timed-out batches without deduplicationTesting
vendor/bin/phpcs(WooCommerce coding standards)Related Issues
🤖 Generated with Claude Code